Any comments @MarkBaker?

I am rebasing and fixing the checks and conflicts whenever we are upgrading this dependency in our project. Let me know if you are going to do a review.

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
If this is still an issue for you, please try to help by debugging it further and sharing your results.
Thank you for your contributions.

@MarkBaker @oleibman anything wrong with this pull request? Don't get why you guys never respond but still the repo is fully active. Seems to be over 200 other pull requests merged since this PR was opened.

My apologies, I do not have sufficient expertise in the area of PHP performance to properly evaluate your change. I will try to read up on it, but it will take a while.

@oleibman Thanks. I will rebase and take a second look at the logic myself. Been a while.

EDIT: Seems like I messed up this pull last time I rebased.

The work done in a189d93 messed up my PR. Need to refactor a bit.

PR updated. You can test the effect by using the snippet from the linked issue.

The effect from 0.9 to 0.3s might not seem huge, but we generate some really large spreadsheets, and this patches reduces time taken by several minutes.

Note: PHPStan bug encountered. $newStyle is actually always defined. Not sure what you guys would prefer to do. Either ignore phpstan message or add if $newStyle === null {. Please let me know.

You have a php-cs-fixer problem as well. I believe the phpstan-var statement at line 81 in Style.php needs to be followed by a blank line (well, a line with just an asterisk in column 6) to eliminate the problem.

As for the phpstan problem, I don't yet understand what you're doing well enough to make a recommendation. In the code before you changed it, newStyle was always assigned a value (as a clone of style). With your change, it is only assigned in the first part of an if ... else ... statement. So, Phpstan's analysis is correct - newStyle may be unassigned. You could eliminate the phpstan problem by moving the assignment to newStyle before the if ... but this might affect your performance numbers if you have to make a lot of unused clones. The other logical alternative is to assign null to newStyle before that if, and then test if newStyle isn't null before using it later. That should not affect your performance much, but I'm not completely convinced that doing nothing in this case is the correct action. It probably is, especially if you can convince me that newStyle will always be non-null here.

I don't yet understand what you're doing well enough to make a recommendation

Not sure how I can help you here. The comments is not enough?

if you can convince me that newStyle will always be non-null here.

Remember, $newStyle is only used if $existingStyle is falsy.

1. This is OK. $newStyle is always set, regardless of $existingStyle.
2. In the else, $newStyle is set if $existingStyle is null.

Now, there is no way you could end up with $existingStyle beeing falsy AND $newStyle not defined. phpstan/phpstan#5805

Note: The code is changed from the one in the picture. In that state, $existingStyle could be false and $existingStyle === null would fail to catch that.

Okay, you've convinced me. You still need to convince Phpstan. Try this just before the addCellXf at the end of your block:

$newStyle = $newStyle ?? new Style();

This will add minimal overhead, and Phpstan should be satisfied.

Okay, we could ignore the error, but I will create a suggestion. I think the best approach would be to do same as we do in "1" from the screenshot, so the style is actually applied.

@oleibman The $newStyle issue should now be fixed.

My own test failed 🤦🏻‍♂️ Fixed now. Rebased on laster master and moved my changelog into "Fixed" instead of "Changed".

Thank you for your patience. As you probably have guessed by now, there are only three guys on our team, and we all do this in our spare time. So it can take a bit of time until we can process PRs. But quality PR like yours do end up being merged, sooner or later.

On a side note, for your next PR, you might want to consider to break (existing) things in smaller methods. We already have some extremely huge methods, and we'd rather try to avoid that in the future. In this specific case I would probably have written a small class dedicated to cache, to better clarify concerns of each parts, and keep Style.php smaller.

Sorry, but I think that this PR and eventually v1.19 has broken a few things.

Please check the reported issue here: #2366